gh-143689: Fix BufferedReader.read1 leaving object in reentrant state on error #143690
Conversation
BufferedReader.read1() could leave the buffered object in a reentrant (locked) state when an exception was raised while allocating the output buffer. This change ensures the internal buffered lock is always released on error, keeping the object in a consistent state after failures. Signed-off-by: Yongtao Huang <yongtaoh2022@gmail.com>
cmaloney
left a comment
cmaloney
left a comment
There was a problem hiding this comment.
Thanks for the patch! The fix looks correct. The NEWs entry and test don't quite match the preferred patterns in CPython, left some comments.
Misc/NEWS.d/next/Library/2026-01-11-14-14-19.gh-issue-143689.fzHJ2W.rst
Outdated
Show resolved
Hide resolved
…zHJ2W.rst Co-authored-by: Cody Maloney <cmaloney@users.noreply.github.com>
Thanks for the review and feedback — I’ve made the requested changes. |
| @unittest.skipUnless(sys.maxsize > 2**32, 'requires 64bit platform') | ||
| @unittest.skipIf(check_sanitizer(thread=True), | ||
| 'ThreadSanitizer aborts on huge allocations (exit code 66).') | ||
| def test_read1_error_does_not_cause_reentrant_failure(self): |
There was a problem hiding this comment.
Does it need @bigmemtest decorator?
There was a problem hiding this comment.
Thank you. I don’t think @bigmemtest is needed here, since the goal is only to trigger the “request too large → failure” error path, not to actually allocate a large amount of memory.
| self.assertIsInstance(cm.exception.__cause__, TypeError) | ||
|
|
||
| @unittest.skipUnless(sys.maxsize > 2**32, 'requires 64bit platform') | ||
| @unittest.skipIf(check_sanitizer(thread=True), |
There was a problem hiding this comment.
Only the non-free-threaded TSan build failed; the UBSan and free-threaded TSan builds are both OK.
Please refer to https://github.com/python/cpython/actions/runs/20896527108/job/60035968975?pr=143690
Lib/test/test_io/test_bufferedio.py
Outdated
|
|
||
| with self.open(os_helper.TESTFN, "rb", buffering=0) as raw: | ||
| bufio = self.tp(raw, buffer_size=8) | ||
| huge = 10**18 |
There was a problem hiding this comment.
I would love to see a comment, why this number is used :)
What are the rules for it?
There was a problem hiding this comment.
Thanks for the suggestion — I’ve switched to using sys.maxsize and added a comment explaining the rationale.
Co-authored-by: sobolevn <mail@sobolevn.me>
3 participants
BufferedReader.read1() could leave the buffered object in a
reentrant (locked) state when an exception was raised while
allocating the output buffer.
This change ensures the internal buffered lock is always released
on error, keeping the object in a consistent state after failures.